XPC service for Lion Sandboxing #165
Conversation
This fixes sparkle-project#163. It requires that you copy the XPC service into your main application bundle in Contents/XPCServices and runs the remove from quarantine step of copying the finish_installation app as well as launching finish_installation app outside of the sandbox. Sparkle will function exactly as it does now for applications that do not copy the XPC service into their application bundle.
|
Very exciting! I'll try to make time this week to review and integrate.
|
|
Thanks very much for the patch. I have made only a first pass at review, as I am seriously writing this from a beach on Maui. I don't think that the launch_task command of the service can be permitted to be so general. An exploit in the host app could use this to execute an arbitrary task outside the sandbox—no good! I think this could be remedied by expressing the path to finish_installation in relative terms from the service. We'd also need to ensure something like the path being installed and the path being updated are signed with the same code signing identity as the parent app. I'm also concerned about the usage of synchronous XPC messages. We can't block the main thread of the client app in that way. While I don't presently have time to handle these issues myself, I would be happy to assist you in doing so if needed. Thanks again for taking the time to do this. |
|
I'll try to work on some of these pieces when I get time. I've been pretty busy recently, though. Any help would be appreciated! Some thoughts: How should the launch_task command be specified relatively? Should there be just a single argument passed to the service that is the name of the finish_installation app (since it could be renamed) and allow the launch_task to figure out the path to the application support folder? I think this should mostly work, but are there cases where you can customize things that might change which bundle is used for calculating the application support folder name (or anything like that)? The code signing part makes sense as well, but is there a good way to extract information about who signed a binary? All I know is In testing the synchronous XPC messages weren't really a problem. I guess there's a possibility of a hang, but there's also file IO going on in the same method which could hang as well. I can't test right now, but is this method running on the main thread anyway? |
|
On Apr 3, 2012, at 8:33 PM, Whitney Young reply@reply.github.com wrote:
I think all this means we need to ensure that the client application copies finish_application into a path which can be computed by trusted information. Also: finish_installation's binary should match a compile-time checksum (the client app is not trusted!). I suppose that if we have this part, we don't necessarily need the fixed-path part.
The relevant method is indeed running on the main thread. I guess the more significant point is that the _sync variants of the XPC APIs are effectively deprecated and are intended only for migratory (i.e. from MIG) applications. |
|
Seems good. I'll take a look when I get a chance. It seems the better way to go would be to verify that the finish_installation binary is code-signed with the same key as the client application. This will provide better security than using a checksum (which would still allow someone to pad an executable to match the hash). At that point, the path tot the executable doesn't really matter, would you agree? I'll take a look at the API and try to get that together. I didn't know that the _sync variants were deprecated. Thanks for letting me know. I'll refactor and get that fixed for sure. |
|
Hello,
|
|
Fraser, Good points. Since the only XPC calls are in a function returning |
|
Did this work get any further? Happy to try to help, but it would be handy to know that I'm not duplicating effort. |
|
Looking at the code, it seems like installWithToolAndRelaunch should be easy enough to split into two, with the completion of the copying triggering the second part. However, there are two issues. First, the existing file copying code is effectively synchronous, so I'm not convinced that there's an absolute requirement to make the xpc one async. I guess that this is a good thing... Second, the last thing that the routine does is to terminate the app. If this happens before the launch task has completed, there might be an issue with the xpc service - are we sure that it will survive the app that launched it going away? Perhaps more to the point, if we make either of the xpc calls asynchronous, that means that the original installWithToolAndRelaunch needs to return without having called NSApp terminate. This presumably has the potential to change the behaviour of whatever called it, which might lead to all sorts of mess. Anyone got any comments? Are my concerns relevant - bearing in mind that I don't know this code base at all! |
|
Sam, I think you'd want to terminate after the XPC service completed. I believe that'd be possible, but this was the first time that I worked with XPC, so I don't know for sure. Other than that I think Andy would know better because he'd be more familiar with the code base. I probably won't be finishing the work on this now, though. We just recently decided that the App we were using this for with sandboxing will be only sold through the Mac App Store now. |
|
Thanks Whitney. I think you're right, the main code should wait until the XPC service completes. This means that installWithToolAndRelaunch would return before the launching had completed. Hopefully that won't cause problems, but maybe Andy can say. |
|
w.r.t. the security-in-XPC-service issue, I expect that the new bits in https://github.com/andymatuschak/Sparkle/tree/CodeSigning should help a great deal! |
|
It's true that making the XPC calls asynchronous will change the code flow somewhat, but I don't think that will be particularly troublesome to any of the callers. |
|
Can I help out on getting this working and secure? I've got some time available in the next few weeks. |
|
@lwdupont go for it - I was planning on having a look at it next week, but I won't have time before then, and something else will probably come up to distract me! |
|
@samdeane Ok, thanks. I'll probably give it a start tomorrow morning. |
|
I changed wbyoung's patch to make the XPC calls asynchronous (commit here). A few issues remain:
|
|
Ok, so now I put the networking code in an XPC as well. This means that Sparkle will not require any extra entitlements; the Sparkle Test App runs without any entitlements at all. You can find the branch here. I've also made a few config changes to get Sparkle to build in Xcode 4.4 on 10.7; my git-fu is too weak to split this up after the fact, sorry. So, how does it work?
Limitations:
|
|
Hi Erik, I'm going to give this a go for a new beta build of Ambientweet. Anything I should watch out for? |
|
Just tested this in the wild. Running a debug build under Xcode worked fine - it successfully downloaded and installed. However, running a release build outside of Xcode resulted in this: Process: Ambientweet FastSpring [57842] Date/Time: 2012-08-22 18:19:30.615 +0100 Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_BREAKPOINT (SIGTRAP) Application Specific Information: Thread 0 Crashed:: Dispatch queue: com.apple.main-thread |
|
Do you see any sandbox violations in Console? |
|
Ah, I'm an idiot :) I didn't add the XPC services to my application... |
|
It appears to be working now. If you download http://downloads.elegantchaos.com/ambientweet/ambientweet-v1.1.2b2.zip, it should try to update to 1.1.2b3. Seems to work for me, but it would be good to hear if it's broken for others :) |
|
My version of @erikaderstedt's branch is samdeane/Sparkle branch:xpc-service (here) It's pretty much identical at this point. |
|
prodding andy to evaluate the submitted patch/pull request to see if it meets his approval for inclusion in master |
|
Erik, thanks so much for your work here! I've reviewed your branch, and aside from cosmetic issues, I have a few questions and a few concerns:
I may have time this or next weekend to try to help finish this off, if that'd be useful. Thanks again for your contributions here! |
|
Hi! Thanks for taking the time to review the branch! To answer your questions: My main motivation for supporting Sparkle in a sandboxed app isn't security; I just want to maintain two versions of my app; one on the App Store and one outside. Preferably, they should be as similar as possible, and that includes sandboxing. That said, it is certainly possible to move the integrity check to the XPC service. I'd love to do it, but it would have to wait until after the next version of my app is launched in a few months (hopefully). libcurl vs. NSURLDownload - good question. The SandboxedFetch sample from Apple uses libcurl, and that's what I used as a starting point. The main function call that actually fetches the file, curl_easy_perform, is synchronous, which is the main difference between libcurl and NSURLDownload. Not very difficult to work around, I guess. If a lot of people are using "rich" release notes (with Javascript, for example), it would be possible to make a WebDataSource that uses the download_service XPC. This feels like it would take a lot of work. One alternative would be to open these release notes in the user's default browser using NSWorkspace. You're right, the file descriptor is not needed. This is "cruft" left over from SandboxedFetch, where the download was made to a temporary file created and named by the XPC service. Since we're now passing the destination file path instead, this is superfluous. I really, really, tried to use the same XPC connection for progress updates, but wasn't able to get it to work. Finally I gave up and did it the same way SandboxedFetch does it. I still don't understand why it does it this way. Limiting the XPC services using sandboxing seems like a good idea! Cheers, 5 sep 2012 kl. 07:47 skrev Andy Matuschak:
|
|
Thanks for the answers, Erik. I understand now why you weren't too concerned about the security ramifications—that makes sense. I'm thinking about it more for apps which never ship on the App Store, but which are still sandboxed; they should still be secure! I'll try to make time to keep iterating on this soon. |
|
(Since it feels the behavior I've seen is a bug – I'd either expect launching the app in its own security session to be invalid and thus it shouldn't appear as a running app in the user's login session; or that is completely valid and thus com.apple.security.pboxd should not appear as a second, apparently broken instance of the launched app – I filed a bug with Apple. radar://12648274, for those who care and can view.) |
|
Hm, but that means that the XPC service can't have a disjoint set of permissions from its host… |
|
Hello! Thank you for providing a solution for this issue with the Sandbox, but I still can't manage to make my app update with the Sandbox. What are the steps that should I follow exactly...? Thank you. |
|
@andymatuschak I want to say (though I may be mis-remembering) that we tested Sparkle updating with our app being sandboxed and the Sparkle XPCServices being signed but not sandboxed and we were able to successfully upgrade, so that would imply that disjoint permissions are allowed even though the security sessions are different. @ClaudiuD Did you have a server running which was hosting the appcast? The solution I described earlier is definitely still working. We're using it in our app right now. |
|
Thank you for the response. No, what files should I add to my server? I am happy that your solution works, because I really need to implement it on my app. |
|
The README.md file in https://github.com/ryannielsen/NSDocumentSandboxSparkleTest describes how to create the test… read the last two paragraphs and the step-by-step instructions at the bottom. In a nutshell, you need
Without those three things, the test will fail. |
|
I implemented this solution to my project too and I respected all the steps, but still that error. |
|
So start again.... I take the Sparkle project from this location https://github.com/tumult/Sparkle (is this the right version?) and I copy in the folder Sparkle from your test app. I change the server base url path constantes and current version of project to 2 and build, but not Appcast folder on the desktop.... I am doing something wrong? |
|
Can you please tell me the steps I need to follow for integrating this Sparkle on my project. I don't know what could be wrong... |
|
On console I get, for my project, not the test project: Its clearly that I don't have permissions to write, because of the Sandbox, but how this service starts on non-sandbox.... |
|
Hi @ClaudiuD this isn't the proper venue for diagnosing what's going wrong with my example project. I'm also afraid I cannot offer support for the project. I created it to ensure the changes in this pull request would meet our needs for our project, nothing more and nothing less. A quick google on "diskimages sandbox" returns http://stackoverflow.com/questions/9119191/sandbox-and-nstask, which then led me to search devforums.apple.com. At the bottom of this thread, you'll see what's causing problems: https://devforums.apple.com/message/745710 It seems that sandboxed apps won't be able to use dmgs as their archive and distribution mechanism. The Sparkle changes it tests work for us on our app which is sandboxed on 10.7 and 10.8, but we distribute zip files. |
|
Thank you for you answer. You save me a lot of time and nerves :) . Best. |
|
This Sparkle works with zip and in my project too. Great job. |
|
@andymatuschak: would it be worth making a branch for this stuff on your repo, if you don't have the time to actually review the changes and merge them into your master branch? That way people have an obvious place to go to get the latest sandbox-friendly version. At the moment it's a bit confusing - you have to read all the way through this thread to work out which commit to take from where. I've got a "sandboxing" branch on my fork which I'm going to attempt to use for the latest version of Neu, but I'm a not 100% certain at the moment that I've picked up everything I should :) |
|
On Feb 8, 2013, at 6:59 AM, Sam Deane notifications@github.com wrote:
|
|
Ok, that makes sense, and I wasn't meaning to imply that you were slacking ;) It looks like some additional work has been done since you posted those comments. If we could collectively figure out a list of jobs still to be done to get it to an acceptable state for merging, then that would increase the likelihood of one of us picking up the baton and doing those jobs. Personally, I'm in a similar position to Erik. I want to sandbox the non-App store version of my app, just for the sake of parity with the App store version, and so that I don't have to maintain two wildly different code-paths. Of course I'd like it to be secure ultimately, but getting a new version of my app out there is higher priority in the short term. Of course, as long as Sparkle is working, I can update the app as Sparkle's sandboxing support gets improved. |
|
It seems to me that one of the main vulnerabilities could be eliminated by simply not passing the path of finish_installation to the XPC service. That stops it from being an arbitrary copying tool. Can't the service figure that out for itself (it should be a fixed relative path from the xpc service's own bundle location, as Andy mentioned in an earlier post). For the other vulnerability, is it enough to have finish_installation verify that the package that it's going to install, the package it's going to replace, and itself were all signed by the same entity? This seems like it ought to be a way to generalise the code so that no information needs to be passed in other than the location of the package to be installed. An exploited app can do whatever it wants with the package or the path to it, but it won't be able to make these signatures match. What am I missing? |
|
It seems to me that an even simpler approach would be to launch the finish_installation tool in the downloaded bundle directly. I don't think it would have a problem with copying the application into place (including itself) even though it's running. The only problem it would have would be in deleting the downloaded bundle afterwards, since it would be inside it. However, this step could just as easily be performed by the updated app, the next time it launched. I'm probably missing something with this idea, but it would eliminate the need for an xpc helper at all. |
|
FWIW, I also wonder if this might be the point to perform some more radical pruning of the code. I added an issue: #248 |
You can't launch an app in a way which would inherit the parent's sandbox. |
|
Is there a fork of Sparkle that supports Sandboxing with CodeSigning right now? I could really use this in my project. I'm distributing a private beta of an app to beta testers and this would be really helpful. |
Pull request here: sparkle-project#165 Changes here: wbyoung@dbd2b9b
…merge Merging in Sam Dean's sandboxing branch which contains the work to get Sparkle to work with sandboxing done by several folks in this pull request thread: sparkle-project#165 Resolved with changes from upstream
|
Because this branch is very old and has security issues (#165 (comment)) — I'm closing this pull request. I realize that sandboxing support is very important and it's a high priority issue, so I'm starting a discussion in #363 to work out how can we restart this effort with a more secure approach. |
This fixes #163. It requires that you copy the XPC service into your main application bundle in Contents/XPCServices and runs the remove from quarantine step of copying the finish_installation app as well as launching finish_installation app outside of the sandbox. Sparkle will function exactly as it does now for applications that do not copy the XPC service into their application bundle.